RMT Onewire Peripheral #454

DaneSlattery · 2024-07-12T13:05:57Z

#445 related

test commit 2 next device asd

Vollbrecht

Thanks for working on this !

I left a couple notes, though overall i think first try to convince the CI, to let the project build for every target. If you succeeded at that point we can go into more details.

bindings.h

.devcontainer/devcontainer.json

src/peripherals.rs

src/onewire.rs

components_esp32.lock

Vollbrecht · 2024-07-12T15:33:05Z

You also get one additional cfg flag that you can use to gate the complete onewire bus implementation. When the remote component is included in the build system it will make the following available #[cfg(ESP_IDF_COMP_ESPRESSIF__ONEWIRE_BUS_ENABLED)] (in rust its all in lower case though ;D )

DaneSlattery · 2024-07-13T12:12:03Z

@Vollbrecht I think I need to also prevent the component from being included if the esp-idf version is 4, is there a flag for that?

Remove references to the ds18b20 specific device. Add a feature flag `rmt-legacy` that, when enabled, will build the original rmt modules from v4 of the esp-idf. When disabled, the v5 rmt interface can be used for one wire applications. Implement the Iterator trait for a device search and use the Iterator in the example.

DaneSlattery · 2024-07-13T22:16:30Z

Struggling to get the v4 builds to go, I feel like it's an issue in the component inclusion that needs to be feature gated for versions <4, but I'm not 100% sure how to do that. I've gotten the v5 builds working across the board though, and I've tested the code is functioning locally on my esp32.

Vollbrecht · 2024-07-14T09:35:12Z

Struggling to get the v4 builds to go, I feel like it's an issue in the component inclusion that needs to be feature gated for versions <4, but I'm not 100% sure how to do that. I've gotten the v5 builds working across the board though, and I've tested the code is functioning locally on my esp32.

The core problem is that the external onewire_bus crate does not support esp-idf v4. And that is fine as we already deprecated the usage of such, so a new API doesn't need to also work there. It just shouldn't break the usage of the crate. The problem arises duo to you directly adding the onewire_bus inside the Cargo.toml of esp-idf-hal, and thous telling the buildsystem to include it, though it can not work with that crate version.

The solution to the problem looks as follows. You are already using the cfg flag that is generated when [[package.metadata.esp-idf-sys.extra_components]] is included. That is halve the problem solved. The rest is solved by not including the metadata section inside the default Cargo.toml of esp-idf-hal. By including it here it will always be pulled in, even if a user is not using the driver. The correct place for it to be included is at the binary level, e.g in the final binary a user is using.

With the inclusion of the headers in esp-idf-sys a user also will only need to add this:

[[package.metadata.esp-idf-sys.extra_components]]
remote_component = { name = "onewire_bus", version = "^1.0.2" }

and not specify any headers himself.

For the example its a bit annoying. To run it you would have to provide a modified Cargo.toml that includes it only for that example. Annoying but well i think without using something different than cargo itself unavoidable. You can add instructions in the doc header of the example how to run that example. It would look like cargp run --example <example-name> --manifest-path <Cargo.toml that includes the metadata.esp-idf-sys section> In the future that potentially will be simpler for examples with the inclution of cargo-script.

TLDR:

put metdata.esp-idf-sys only in your final binary to signal that you want to use it
for the example you need to pass a cargo.toml with the --manifest key path to a cargo.toml that includes it.

Cargo.toml

src/lib.rs

bindings.h

DaneSlattery · 2024-07-14T15:27:46Z

@Vollbrecht unfortunately we need both includes. I've made a PR into esp-idf-sys. esp-rs/esp-idf-sys#325. It does work with the code not inside a module. Are you against namespacing inside the bindings?

DaneSlattery · 2024-07-14T15:40:04Z

I have an issue now in that the old RMT examples will never get compiled and may actually be broken. Perhaps we should add a CI stage to test with the legacy_rmt and legacy_oneshot_adc flags.

Vollbrecht · 2024-07-15T13:40:42Z

I have an issue now in that the old RMT examples will never get compiled and may actually be broken. Perhaps we should add a CI stage to test with the legacy_rmt and legacy_oneshot_adc flags.

its broken, because of the problem i stated in my last post. For idf v4 the old driver is always used, and don't need a flag because v4 only has the old driver. You can't include the onewire_bus inside v4, That's why it need to be moved from the esp-idf-hal Cargo.toml to the binary Cargo.toml

DaneSlattery · 2024-07-15T17:45:16Z

its broken, because of the problem i stated in my last post. For idf v4 the old driver is always used, and don't need a flag because v4 only has the old driver. You can't include the onewire_bus inside v4, That's why it need to be moved from the esp-idf-hal Cargo.toml to the binary Cargo.toml

Let me put it another way. Even for idf v5, the rmt-legacy and adc-oneshot-legacy features are disabled in all of our tests because we don't do any tests with those features activated. We do a build for no_std and alloc, but not for these other features, which means we aren't ensuring that these examples will still work.

Vollbrecht · 2024-07-18T20:01:44Z

gj making the first milestone -> getting CI green ;D

I am a bit busy right now, but will have a closer look at the weekend and hope to have some more feedback by then.

ivmarkov · 2024-07-23T06:57:50Z

@Vollbrecht @DaneSlattery This had been going on for quite some time. I'm not sure in the meantime - is this ready for merge, and what (if any) are the outstanding issues?

DaneSlattery · 2024-07-23T07:24:56Z

Hi @ivmarkov

The build works and the code is tested. The outstanding items are this:

The rmt-legacy and adc-oneshot-legacy features are disabled in all of the CI/CD tests because we don't do any tests with those features activated. We do a build for no_std and alloc, but not for these other features, which means we aren't ensuring that these examples will still work.
There are a few comments of mine, I will @ you on them. I have not clicked "resolved" on any of @Vollbrecht comments, but I have done them.
I am not sure if this part needs to become a `peripheral``.
I need to create a follow-up PR using this OneWire interface for a real device.

src/onewire.rs

ivmarkov · 2024-07-23T13:08:55Z

src/onewire.rs

+ ///
+ /// The pin will be used as an open drain output.
+ pub fn new(
+ pin: impl Peripheral<P = impl crate::gpio::InputPin + crate::gpio::OutputPin> + 'd,


Since internally the ESP IDF onewire driver uses the new RMT driver (a single channel for TX/RX) (plus the pin thst you already track) you need to somehow track the usage of the RMT channel peripheral, as it is still a peripheral - just like the pin which you track already.

If you don't track it, nothing major would happen because the old and the new RMT driver cannot co-exist, yet would still be good to follow the pattern established with the other drivers.

Now, some of the "new" (ESP IDF 5+) C drivers no longer take a peripheral "id"/"number". For example, the old RMT driver used to take a channel number, but the new C RMT driver doesn't as it internally manages what channel to be used. If you try to instantiate more driver instances than hardware channels you have available, it would likely return a runtime EspError. So in a way - with the new RMT C driver you don't really know which concrete hardware channel peripheral will be used by that particular C driver instance.

With that said, nothing stops us from continuing the old way and passing to the driver a RMT "peripheral" (note that you don't need "new" RMT channel peripherals for the new C RMT driver, because the peripherals are ghost structures which however model actual hardware on the chip and not higher-level C driver concepts).

So what I suggest is to just enhance the new constructor by passing an impl ... Channel ... peripheral that you can copy from the old RMT driver wrapper. While there is no warranty that the C driver will use exactly the same channel that you pass to it - we don't care, as the channels are indistinguishable from each other. We just use - with the new C driver - the channels as a way of glorified "reference counting" of sorts.

(Just mentioning that this comment ^^^ is still valid.)

Yeah I had planned to look into this. It becomes quite problematic as you mentioned below because I ifdef the whole rmt module.

Well I would not call it "quite problematic". #ifdef-ing only the driver inside rmt module is hopefully not a rocket science. Easiest way to do it is what I suggested for the examples:

Define a mod driver {} internal module (not public) - inside the rmt module itself. no need for a separate file, just an inline module.

Move all code except the peripheral! peripherals' definitions as well as the RMT peripheral traits (if any) inside the driver module

Put a cfg on top of the driver module

Put a cfg-ed pub use driver::* in the rmt module

No big deal?

No big deal. It's done.

src/onewire.rs

ivmarkov · 2024-07-23T13:23:00Z

Hi @ivmarkov

The build works and the code is tested. The outstanding items are this:

The rmt-legacy and adc-oneshot-legacy features are disabled in all of the CI/CD tests because we don't do any tests with those features activated. We do a build for no_std and alloc, but not for these other features, which means we aren't ensuring that these examples will still work.

There are a few comments of mine, I will @ you on them. I have not clicked "resolved" on any of @Vollbrecht comments, but I have done them.

I am not sure if this part needs to become a `peripheral``.

I need to create a follow-up PR using this OneWire interface for a real device.

I think at least a sketch of how the read/write API is needed - and I would say - as part of this PR rather than as a follow-up PR, because - depending how RW works (would you place the read/write methods on the "device" structure or on the "bus" structure?) this leads to one or another design.

The other big problem is how dynamic are the devices on the bus. Are they somehow static or can they come and go (say - by physically removing the device from the wire)? Even if they can come and go, how often does that happen?

ivmarkov

@DaneSlattery I think we are getting there!
I've left a lot of comments, but these I hope would be quite easy to fix, as the latest approach seems solid.

My biggest request is to remove OWDevice completely, as I continue to fail to understand what purpose it serves? Please let me know if I'm missing something.

.cargo/config.toml

.gitignore

Cargo.toml

src/onewire.rs

ivmarkov · 2024-07-28T11:06:32Z

src/lib.rs

+#[cfg(all(feature = "rmt-legacy", esp_idf_comp_espressif__onewire_bus_enabled))]
+compile_error!("the onewire component cannot be used with the legacy rmt peripheral");
+
+#[cfg(any(feature = "rmt-legacy", esp_idf_version_major = "4"))]


This cannot be done so simply. As discussed during the previous iteration, you do need the RMT peripherals which are defined by the esp_idf_hal::rmt module. And you need to pass a RMT channel to OWDriver in addition to the pin. So you can't just #ifNdef the rmt module when the rmt-legacy feature is not defined. What you need to do instead is to leave the rmt module here unconditionally, and then - inside the rmt module - protect with an #ifdef all of the driver code, but leave the peripheral definitions always defined and valid - even when the legacy RMT driver is #ifNdef-ed.

ivmarkov · 2024-07-28T11:28:16Z

src/peripherals.rs

@@ -10,6 +10,7 @@ use crate::mac;
 use crate::modem;
 #[cfg(any(esp32, esp32s2, esp32s3, esp32c6))]
 use crate::pcnt;
+#[cfg(any(feature = "rmt-legacy", esp_idf_version_major = "4"))]


As per above, remove the cfg line. The code in the esp_idf_hal::rmt module should be changed so that the RMT peripherals are always defined. Even if the legacy RMT driver is #ifNdef-ed.

ivmarkov · 2024-07-28T11:28:24Z

src/peripherals.rs

@@ -71,6 +72,7 @@ pub struct Peripherals {
 pub ledc: ledc::LEDC,
 #[cfg(esp32)]
 pub hledc: ledc::HLEDC,
+ #[cfg(any(feature = "rmt-legacy", esp_idf_version_major = "4"))]


ivmarkov · 2024-07-28T11:28:31Z

src/peripherals.rs

@@ -168,6 +170,7 @@ impl Peripherals {
 ledc: ledc::LEDC::new(),
 #[cfg(esp32)]
 hledc: ledc::HLEDC::new(),
+ #[cfg(any(feature = "rmt-legacy", esp_idf_version_major = "4"))]


ivmarkov · 2024-07-28T11:43:56Z

@DaneSlattery Unrelated to the implementation chosen in this PR, but people do seem to use UART successfully as well to implement onewire in hardware: https://developer.electricimp.com/resources/onewire

Interesting!

ivmarkov · 2024-08-05T08:13:13Z

@DaneSlattery No rush, but wondering if you still plan to fix the remaining issues?

ivmarkov · 2024-08-05T08:15:22Z

Shall I again re-review? I see you have marked with "Done" most (if not all) of the outstanding issues?
Weird - I have NOT received an email notification for that... :)

ivmarkov · 2024-08-05T08:17:35Z

Actually, no - there are still unaddressed outstanding issues...

DaneSlattery · 2024-08-05T08:20:06Z

Hi Ivmarkov.
I have addressed most of the comments, but I am still planning to work on

reworking the affected examples so that there is only one ifdef
reworking the legacy RMT feature flag to allow for channel peripherals

I am happy for you to mark as accepted the tasks that were done so far

ivmarkov · 2024-08-05T08:26:15Z

Hi Ivmarkov. I have addressed most of the comments, but I am still planning to work on

reworking the affected examples so that there is only one ifdef

reworking the legacy RMT feature flag to allow for channel peripherals

Np - take your time. I just wasn't sure what is the status quo, and as mentioned I somehow did not receive notifications for the already addressed tasks.

I am happy for you to mark as accepted the tasks that were done so far

I just did - thanks!

src/onewire.rs

Moves the RMT driver into a private `driver` module, which will only be compiled in rmt-legacy mode. Also reduce the number of cfg feature flags in examples by wrapping the implementation in a module.

DaneSlattery · 2024-08-09T13:27:41Z

Done! I somehow don't think this CI failure is exactly my fault, not sure what's happening.

Vollbrecht · 2024-08-09T14:48:46Z

Done! I somehow don't think this CI failure is exactly my fault, not sure what's happening.

Thanks for continue working on this! Yeah currently they broke nightly for all users that are using the build-std feature though it seams a patch fixing it was already merged. So should hopefully working again tommorow :D

ivmarkov · 2024-08-09T15:27:56Z

Yes let's wait until tmr when we would hopefully have a clean CI, which would ensure the latest changes in this PR do not introduce build errors we have to fix post-merge.

DaneSlattery · 2024-08-10T12:27:22Z

Greeen!

ivmarkov · 2024-08-10T13:11:25Z

Thank you for your persistence through this exercise. Merging.

Post merge, I might exchange the order of channel and pin in OWDriver::new because everything else has it with the peripheral first and pins following, but that's all there is to it that I see as a follow up. Great job!

Dane Slattery added 2 commits July 12, 2024 14:26

commit test 1

a37af47

test commit 2 next device asd

try example, haven't built yet

d45b33f

Vollbrecht requested changes Jul 12, 2024

View reviewed changes

bindings.h Outdated Show resolved Hide resolved

.devcontainer/devcontainer.json Outdated Show resolved Hide resolved

src/peripherals.rs Outdated Show resolved Hide resolved

src/onewire.rs Outdated Show resolved Hide resolved

components_esp32.lock Outdated Show resolved Hide resolved

DaneSlattery changed the title ~~Dms rmt onewire periph~~ RMT Onewire Peripheral Jul 13, 2024

This comment was marked as resolved.

Sign in to view

DaneSlattery marked this pull request as draft July 13, 2024 12:11

DaneSlattery added 7 commits July 13, 2024 15:22

Add more feature guards to examples compile

b9de421

Add alternative main functions for examples

cc879bf

Use std threading

0183797

Add std to thread sleep

6e3f9b0

use std duration

7148db1

correct use for legacy rmt component

719911b

DaneSlattery marked this pull request as ready for review July 13, 2024 22:15

Vollbrecht requested changes Jul 14, 2024

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

bindings.h Outdated Show resolved Hide resolved

DaneSlattery mentioned this pull request Jul 14, 2024

Expose onewire device esp-rs/esp-idf-sys#325

Merged

Remove local bindings and use the esp-idf-sys master

c72e53c

fix a typo

cb17743

DaneSlattery added 4 commits July 15, 2024 20:15

Remove esp-idf component inclusion

af7827b

Update gitignore

f2453a4

Fix examples

9696af5

mut-ex out the example compilation

a72fb45

ivmarkov requested changes Jul 23, 2024

View reviewed changes

DaneSlattery added 3 commits July 27, 2024 17:13

add implementation for temperature readings

d0f1185

Conditional compile

389135b

esp4.4 compile

f8145c1

ivmarkov requested changes Jul 28, 2024

View reviewed changes

DaneSlattery added 4 commits July 28, 2024 19:28

Changes to cargo and config

831be2d

Remove OWDevice and simplify API

bcf0ddd

Remove phantomdata that is not required

8a8d7fd

adjust lifetimes for single search at a time

c110fef

ivmarkov reviewed Aug 5, 2024

View reviewed changes

src/onewire.rs Outdated Show resolved Hide resolved

DaneSlattery added 3 commits August 7, 2024 22:37

Keep RMT peripheral and add channel to onewire

a7e26a9

Moves the RMT driver into a private `driver` module, which will only be compiled in rmt-legacy mode. Also reduce the number of cfg feature flags in examples by wrapping the implementation in a module.

modularise examples

fd076e8

Fix for CI

269f4f3

update example name to make it pop

eb6b1c2

ivmarkov merged commit aa0e257 into esp-rs:master Aug 10, 2024
10 checks passed

RMT Onewire Peripheral #454

RMT Onewire Peripheral #454

Conversation

DaneSlattery commented Jul 12, 2024 • edited Loading

Vollbrecht left a comment

Choose a reason for hiding this comment

Vollbrecht commented Jul 12, 2024 • edited Loading

This comment was marked as resolved.

DaneSlattery commented Jul 13, 2024

DaneSlattery commented Jul 13, 2024 • edited Loading

Vollbrecht commented Jul 14, 2024

DaneSlattery commented Jul 14, 2024 • edited Loading

DaneSlattery commented Jul 14, 2024

Vollbrecht commented Jul 15, 2024

DaneSlattery commented Jul 15, 2024

Vollbrecht commented Jul 18, 2024 • edited Loading

ivmarkov commented Jul 23, 2024

DaneSlattery commented Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivmarkov Jul 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivmarkov commented Jul 23, 2024

ivmarkov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivmarkov commented Jul 28, 2024

ivmarkov commented Aug 5, 2024

ivmarkov commented Aug 5, 2024

ivmarkov commented Aug 5, 2024

DaneSlattery commented Aug 5, 2024

ivmarkov commented Aug 5, 2024

DaneSlattery commented Aug 9, 2024

Vollbrecht commented Aug 9, 2024

ivmarkov commented Aug 9, 2024

DaneSlattery commented Aug 10, 2024

ivmarkov commented Aug 10, 2024 • edited Loading

DaneSlattery commented Jul 12, 2024 •

edited

Loading

Vollbrecht commented Jul 12, 2024 •

edited

Loading

DaneSlattery commented Jul 13, 2024 •

edited

Loading

DaneSlattery commented Jul 14, 2024 •

edited

Loading

Vollbrecht commented Jul 18, 2024 •

edited

Loading

DaneSlattery commented Jul 23, 2024 •

edited

Loading

ivmarkov Jul 28, 2024 •

edited

Loading

ivmarkov commented Aug 10, 2024 •

edited

Loading